Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support timestamp skipping in test cases #569

Merged
merged 4 commits into from
Jun 13, 2024

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Mar 7, 2023

Description

support timestamp skipping in test cases

How has this been tested?

test case run by gnodev

@r3v4s r3v4s marked this pull request as ready for review March 7, 2023 09:13
@r3v4s r3v4s requested a review from a team as a code owner March 7, 2023 09:13
@r3v4s
Copy link
Contributor Author

r3v4s commented Mar 28, 2023

This PR is ready, but I just saw #657
If GetTimestamp is going to be remove, we can close this PR

@thehowl
Copy link
Member

thehowl commented Mar 28, 2023

@r3v4s I don't think we need to close it - can we make it use time.Now() instead?

@r3v4s
Copy link
Contributor Author

r3v4s commented Mar 28, 2023

@thehowl I wasn't thinking straight. Even if we get rid of std.GetTimeStamp we can still query timestamp using stdlibs's time package via time.Now()

So maybe we can leave this pr bit longer.

BTW, Can you please specify what you're referring to as "it" in "can we make it"?

@thehowl
Copy link
Member

thehowl commented Mar 28, 2023

The PR. I was just saying you could modify the code to use time.Now instead :)

@moul
Copy link
Member

moul commented Apr 11, 2023

To give you some context on time.Now(), currently it has a fixed value set during the first consensus and added to the transaction's metadata before execution. If you call it multiple times during the same call, you'll get the same value. However, we plan to make it dynamic by using opcodes for determinism. Regardless, it should be possible to replace std.GetTimestamp with time.Now() for the end developer.

examples/gno.land/r/demo/mtimestamp/mtimestamp_test.gno Outdated Show resolved Hide resolved
gnovm/tests/imports.go Outdated Show resolved Hide resolved
gnovm/tests/imports.go Outdated Show resolved Hide resolved
@r3v4s r3v4s mentioned this pull request Jun 19, 2023
18 tasks
@ajnavarro ajnavarro requested a review from thehowl June 19, 2023 10:20
@thehowl thehowl mentioned this pull request Jul 5, 2023
@r3v4s
Copy link
Contributor Author

r3v4s commented Aug 7, 2023

NEW IDEA

Since #969 is merged, now gno will create block every 5 seconds.
How about adding ctx.Timestamp += (count * 5) in TestSkipHeights

gno/gnovm/tests/imports.go

Lines 643 to 656 in 5d3be42

pn.DefineNative("TestSkipHeights",
gno.Flds( // params
"count", "int64",
),
gno.Flds( // results
),
func(m *gno.Machine) {
arg0 := m.LastBlock().GetParams1().TV
count := arg0.GetInt64()
ctx := m.Context.(stdlibs.ExecContext)
ctx.Height += count
m.Context = ctx
},

something like this maybe??

		pn.DefineNative("TestSkipHeights",
			gno.Flds( // params
				"count", "int64",
			),
			gno.Flds( // results
			),
			func(m *gno.Machine) {
				arg0 := m.LastBlock().GetParams1().TV
				count := arg0.GetInt64()

				ctx := m.Context.(stdlibs.ExecContext)
				ctx.Height += count

				ctx.Timestamp += (count * 5)
				m.Context = ctx
			},
		)

@thehowl
Copy link
Member

thehowl commented Aug 24, 2023

Since #969 is merged, now gno will create block every 5 seconds.
How about adding ctx.Timestamp += (count * 5) in TestSkipHeights

I don't think block time every 5s is something set in stone, and I think having ctx.Timestamp += (count * 5) in TestSkipHeights is a bit "untransparent"? (even though it reflects real-world behaviour.)

Furthermore I think TestSetTimestamp (#924) might be a better feature.

@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
@r3v4s
Copy link
Contributor Author

r3v4s commented Sep 12, 2023

@thehowl

  • I don't think block time every 5s is something set in stone > Yep, it isn't set in stone
  • "untransparent"? (even though it reflects real-world behaviour.) > Personally I just prefer for test case to be run as same as possible as real gno-vm. But I do understand this kind of automation can be opaque.

I'll still implement automation, but we can wait for #924

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for gno-docs failed.

Name Link
🔨 Latest commit 93f39c9
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs/deploys/65010fb8db87680008015060

@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related and removed 🧾 package/realm Tag used for new Realms or Packages. labels Sep 13, 2023
moul
moul previously requested changes Sep 22, 2023
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please add a unit test to prevent regressions.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.13%. Comparing base (dd68d61) to head (d4c8776).
Report is 104 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   48.44%   58.13%   +9.69%     
==========================================
  Files         409      389      -20     
  Lines       61965    65275    +3310     
==========================================
+ Hits        30019    37950    +7931     
+ Misses      29446    24503    -4943     
- Partials     2500     2822     +322     
Flag Coverage Δ
gnovm 60.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@r3v4s r3v4s requested review from piux2 and a team as code owners March 18, 2024 10:31
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Mar 18, 2024
@r3v4s r3v4s requested a review from moul March 28, 2024 06:08
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @r3v4s - I see this is a long standing PR with some discussions going on on how we should do this.

To get a v1 on this - let's just simplify it and just add count to ctx.Timestamp. Then, please add a txtar test for this, and we can merge it. We should at least have something, and then iterate, rather than be stuck on a discussion.

Also, please check why examples/test is failing in the CI 🙏

@r3v4s r3v4s requested review from deelawn and mvertes as code owners May 7, 2024 03:45
@r3v4s
Copy link
Contributor Author

r3v4s commented May 7, 2024

@leohhhn

let's just simplify it and just add count to ctx.Timestamp

In this case, skipping 1 block will increase time 1 second. Did I understood correctly?

Then, please add a txtar test for this, and we can merge it.

I don't think I can. AFAIK Test* functions are only callable with gno test not txtar.

// UPDATE:
ci was failing because rand was using time value, fixed in cb8d0a0

@zivkovicmilos
Copy link
Member

@moul I think @r3v4s added a regression test to the PR 🙏

@leohhhn
Copy link
Contributor

leohhhn commented Jun 4, 2024

Hey @r3v4s, I will try to push this forward with @moul. In the meantime, can you please add a sentence to this page to tell users that TestSkipHeight will also move the block timestamp?

@r3v4s
Copy link
Contributor Author

r3v4s commented Jun 5, 2024

Hey @r3v4s, I will try to push this forward with @moul. In the meantime, can you please add a sentence to this page to tell users that TestSkipHeight will also move the block timestamp?

Updated d4c8776

@zivkovicmilos
Copy link
Member

Pinging @moul for a second review 🙏

@thehowl thehowl dismissed moul’s stale review June 13, 2024 16:24

Old, and issues addressed

@thehowl thehowl merged commit a22ce24 into gnolang:master Jun 13, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: 🌟 Wanted for Launch
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants